Token propagation E2E tests. #581

rubenvp8510 · 2019-08-06T04:36:11Z

Signed-off-by: Ruben Vargas [email protected]

~~Also, in my tests I realized that this is not gonna work if the query service has --es.use-aliase flag enabled.~~

rubenvp8510 · 2019-10-10T04:00:26Z

In the end, I haven't needed to move anything on the image, it works if I add to the test openshift user the cluster-admin-role, this does the job because the way the openshift-elasticsearch-plugin works[1].

If the subjectAccessReview is satisfied, it assigns a role to the user, in this case in the origin-logging-elasticsearch5 image there is a jaeger[2] configuration for that. This allows some users to have access to the indices. (and that is why the cluster-admin can do requests :D ). This will allow us to test the whole thing without touch the image.. I think this works at least for testing...

jpkrohling · 2019-10-14T08:25:12Z

Is this ready for review? If so, there are a couple of things that needs to be cleaned up first, like commented out code and the user change in the Makefile.

rubenvp8510 · 2019-10-14T14:26:46Z

@jpkrohling Ahh! there is small details that I need to fix, (one of them is the things you pointed in your comment) ;)

Thanks

rubenvp8510 · 2019-10-15T02:51:39Z

@jpkrohling now is ready

jpkrohling

Overall, looks good to me. There's only one change I'd like to see, about the random string generation. The e2e tests should be reviewed by @kevinearls, and once he gives his approval, mine will follow.

jpkrohling · 2019-10-24T08:04:22Z

pkg/util/util.go

+	_, err := rand.Read(randString)
+	if err != nil {
+		// If we cannot generate random, return fixed.
+		return "ncNDoqLGrayxXzxTn5ANbOXZp3qXd0LA"


Return an error instead, and let the caller decide. At the very least, there should be a WARN in the logs about this. It was really bad that I didn't realize that "SECRET" should have been a real random string...

jpkrohling · 2019-10-24T08:05:12Z

pkg/inject/oauth_proxy.go

+		args = append(args,
+			"--pass-access-token=true",
+			"--pass-user-bearer-token=true",
+			"--scope=user:info user:check-access user:list-projects",


Spaces as separators? That's odd!

jpkrohling · 2019-10-24T08:05:55Z

.ci/start-openshift.sh

 # install and run OCP
 sudo docker cp $(docker create docker.io/openshift/origin:$OPENSHIFT_VERSION):/bin/oc /usr/local/bin/oc
-oc cluster up --version=$OPENSHIFT_VERSION
+
+oc cluster up --version=$OPENSHIFT_VERSION --public-hostname=${HOST_IP}.nip.io


cc @kevinearls

kevinearls

As noted the test does not run for me on OCP 4.2

kevinearls · 2019-10-24T08:51:39Z

test/e2e/elasticsearch_token_propagation_test.go

+		defer resp.Body.Close()
+		return true, nil
+	})
+	require.NoError(t, err, "Token propagation tmake est failed")


kevinearls · 2019-10-24T08:55:45Z

test/e2e/elasticsearch_token_propagation_test.go

+
+	client, err := oAuthAuthorization(queryHost, "user-test-token", "any")
+
+	require.NoError(t, err)


The test is failing for me here when I use a real OCP 4.2 OpenShift cluster. Have you been testing against minishift? I think you probably need to use the actual route here.

I haven't tested against OCP 4.2/minishift, I'll give it a try and fix what is failing. Thanks!

jpkrohling · 2019-12-05T15:03:32Z

.ci/run-e2e-tests.sh

+    oc create user user-test-token
+    oc adm policy add-cluster-role-to-user cluster-admin user-test-token
+    # for ocp 4.2
+    htpasswd -c -B -b users.htpasswd user-test-token any


It's a better idea to just ship the actual file instead of generating the file at the CI.

jpkrohling · 2019-12-05T15:05:00Z

.ci/run-e2e-tests.sh

+then
+    echo "Running token propagation tests"
+    oc create user user-test-token
+    oc adm policy add-cluster-role-to-user cluster-admin user-test-token


The bad thing about using "root-like" permissions in a test is that you might miss the cases where the user might not have the appropriate permissions. Not sure it's relevant for your test here, but in general, we should aim to have the minimum set of permissions possible.

jpkrohling · 2019-12-05T15:15:42Z

pkg/inject/oauth_proxy.go

 	args := []string{
-		"--cookie-secret=SECRET",
+		fmt.Sprintf("--cookie-secret=%s", secret),


This generates secrets containing chars like "+" and "/". How are they translated into the final YAML? Do they cause any troubles? I don't think it does, but it's good to have positive confirmation (and perhaps a test). Example of random strings I got from this code locally:

pZ8F0imysFsTwJ9kF8U92g== ILmvfqWW/+bLEokY06NHig== Zmc+jX2VWXkxsB98AIYwDg== xb/fYVT6UHxYpNJXhjR7RQ==

jpkrohling · 2019-12-05T15:20:15Z

pkg/storage/elasticsearch.go

@@ -38,6 +38,53 @@ type ElasticsearchDeployment struct {
 	Secrets    []corev1.Secret
 }

+func (ed *ElasticsearchDeployment) injectArguments(container *corev1.Container) {
+	container.Args = append(container.Args,


Probably a matter of taste, but do you need to split this line here? The next line seems small enough to fit here.

no I don't need to split the line not sure why I did it, or may be the make format did it for me? Anyway I will change it.

jpkrohling · 2019-12-05T15:21:12Z

pkg/storage/elasticsearch.go

+		container.Args = append(container.Args, "--es.tls=true")
+	}
+
+	container.Args = append(container.Args,


Shouldn't these options be added only if es.tls=true?

jpkrohling · 2019-12-05T15:23:48Z

test/e2e/elasticsearch_token_propagation_test.go

+
+}
+
+func (suite *TokenPropagationTestSuite) TearDownSuite() {


Do you need this func if it's empty? The commented out code should be removed anyway.

Ah this should not be empty, I commented those lines on my local but shouldn't be commented.

jpkrohling · 2019-12-05T15:25:50Z

test/e2e/elasticsearch_token_propagation_test.go

+	}
+
+	err := wait.Poll(retryInterval, timeout, func() (done bool, err error) {
+		req, err := http.NewRequest(http.MethodGet, suite.queryServiceEndPoint, nil)


Check that error is nil, and return early in case of errors (return false, err). Otherwise, it will fail in the next line.

Looks like this wasn't done yet.

jpkrohling · 2019-12-05T15:26:51Z

test/e2e/elasticsearch_token_propagation_test.go

+
+	/* Try to reach query endpoint */
+	err = wait.Poll(retryInterval, timeout, func() (done bool, err error) {
+		req, err := http.NewRequest(http.MethodGet, suite.queryServiceEndPoint, nil)


Same comment as before: make the test more resilient, otherwise it might become annoying to all future PRs ;)

You probably want to check the err objects in this method and log their causes, without hard failing.

jpkrohling · 2019-12-05T15:28:33Z

test/e2e/elasticsearch_token_propagation_test.go

+		Jar: cookieJar,
+	}
+	/* Start oauth */
+	resp, err := client.Get("https://" + host + "/oauth/start")


Kinda of the same as previous comments, but you probably just want to retry in case of some well-known networking problems, like timeouts.

jpkrohling · 2019-12-05T15:31:19Z

test/e2e/elasticsearch_token_propagation_test.go

+	var req *http.Request
+	/* Submit form */
+	if hasForm(responseBytes) {
+		req = getLoginFormRequest(responseBytes, resp.Request.URL, user, pass)


For OpenShift, do you really need to parse the form and simulate a real user there? Can't you just send the user:pass with basic auth? I think the Maistra components to send the requests via basic auth.

Well, I can get a token an pass it to query service, but I don't know how to do it and test the whole thing using oauth proxy sidecar, Can we use basic authentication to and set the oauth proxy cookie? May be I'm missing something. Or we don't need to test using the sidecar?

Maistra QE can confirm, but I understand that you can send basic auth and the proxy will do all the token negotiation based on it. Jaeger will see a token, just like if it was a browser session.

Signed-off-by: Ruben Vargas <[email protected]>

Signed-off-by: Ruben Vargas <[email protected]> Signed-off-by: Ruben Vargas <[email protected]>

jpkrohling

LGTM. A couple of test resiliency + negative tests requests, but this could be merged as is and get the remaining comments addressed in a follow-up PR.

jpkrohling · 2020-01-16T09:40:45Z

pkg/inject/oauth_proxy.go

+func proxyInitArguments(jaeger *v1.Jaeger) []string {
+	secret, err := util.GenerateProxySecret()
+	if err != nil {
+		jaeger.Logger().Warnf("Error generating secret: %s, fallback to fixed secret", secret)


WithError(err), so that the cause is logged.

jpkrohling · 2020-01-16T09:42:52Z

test/e2e/elasticsearch_token_propagation_test.go

+	}
+
+	err := wait.Poll(retryInterval, timeout, func() (done bool, err error) {
+		req, err := http.NewRequest(http.MethodGet, suite.queryServiceEndPoint, nil)


Looks like this wasn't done yet.

jpkrohling · 2020-01-16T09:43:17Z

test/e2e/elasticsearch_token_propagation_test.go

+			APIVersion: "jaegertracing.io/v1",
+		},
+	}))
+	assert.NoError(t, framework.AddToFrameworkScheme(apis.AddToScheme, &esv1.ElasticsearchList{


Perhaps you want to use require here instead of assert?

jpkrohling · 2020-01-16T09:46:09Z

test/e2e/elasticsearch_token_propagation_test.go

+
+	/* Try to reach query endpoint */
+	err = wait.Poll(retryInterval, timeout, func() (done bool, err error) {
+		req, err := http.NewRequest(http.MethodGet, suite.queryServiceEndPoint, nil)


You probably want to check the err objects in this method and log their causes, without hard failing.

jpkrohling · 2020-01-16T09:48:13Z

test/e2e/elasticsearch_token_propagation_test.go

+
+	client, err := oAuthAuthorization(host, username, password)
+
+	err = wait.Poll(retryInterval, timeout, func() (done bool, err error) {


Those tests seem to be missing.

jpkrohling · 2020-01-16T09:49:58Z

@kevinearls could you give this a test in OpenShift before this gets merged?

kevinearls · 2020-01-16T10:46:38Z

@jpkrohling Sure, I'll try it later today or first thing tomorrow

kevinearls · 2020-01-16T16:23:13Z

@jpkrohling @rubenvp8510 I've tried this on an OCP 4.2 cluster, but unfortunately have not been able to get it to work so far. The TestTokenPropagationNoToken test passes, but TestTokenPropagationNoToken fails at line 134 with a 403.

I will note a couple of issues I came across in the code, but I don't have any ideas so far on how to get this to work.

rubenvp8510 · 2020-01-16T16:31:31Z

@kevinearls I'll check, TestTokenPropagationNoToken should throws a 403 (Forbidden) , is the expected thing, I'll check other test to see why it does not pass.

kevinearls · 2020-01-16T16:45:19Z

test/e2e/elasticsearch_token_propagation_test.go

+		require.Equal(t, http.StatusOK, resp.StatusCode)
+		if resp.StatusCode != http.StatusOK {
+			return false, errors.New("Query service returns http code: " + string(resp.StatusCode))
+		}


The if is redundant here as if the require.Equal fails the test will terminate. You should pick one or the other. In general I prefer the require as it makes for more concise code and produces better error messages. However, if you prefer the if you need to change the string(resp.StatusCode) to strconv.Itoa

I preferred required too, but I had to do in this way, so if it fails the first time due the pod not fully ready or other timing issues, the wait.Poll will retry again.

If I use required, it will fail mediately. Making this test less reliable. IMHO.

I removed the required line.

kevinearls · 2020-01-16T16:48:59Z

test/e2e/elasticsearch_token_propagation_test.go

+}
+
+func bindOperatorWithAuthDelegator() {
+	roleBinding := rbac.ClusterRoleBinding{


This clusterrolebinding and the token-test-user-bind clusterrolebinding created below need to be deleted at the end of the test (in TearDownSuite()) otherwise the test will fail after the first run as these will already exist on the cluster.

Thanks for the observation, I've added cleaning routine for remove all created roles/role-bindings.

kevinearls · 2020-01-16T16:53:25Z

@rubenvp8510 Sorry, my comment about the test failure was wrong. TestTokenPropagationValidToken is the test that fails, TestTokenPropagationNoToken works correctly.

Sorry about the careless cut and paste on my part. 😧

rubenvp8510 · 2020-01-16T18:31:51Z

I ran the tests a couple of times, and sometimes fails and others not, I'll do this more reliable

Signed-off-by: Ruben Vargas <[email protected]>

kevinearls

LGTM. Works for repeated runs on OCP 4.2

jpkrohling · 2020-01-22T14:02:41Z

Thanks for being persistent, @rubenvp8510!

rubenvp8510 force-pushed the token-propagation branch 2 times, most recently from 0bc6efa to ce44015 Compare October 10, 2019 16:20

rubenvp8510 changed the title ~~[WIP] Add query service token propagation support~~ Add query service token propagation support Oct 14, 2019

rubenvp8510 force-pushed the token-propagation branch 12 times, most recently from 0d315fd to d498565 Compare October 15, 2019 01:00

rubenvp8510 force-pushed the token-propagation branch 9 times, most recently from 768a43d to 273a566 Compare October 24, 2019 02:20

jpkrohling reviewed Oct 24, 2019

View reviewed changes

kevinearls suggested changes Oct 24, 2019

View reviewed changes

rubenvp8510 force-pushed the token-propagation branch from b14bdf9 to d22bdfd Compare December 5, 2019 05:09

jpkrohling reviewed Dec 5, 2019

View reviewed changes

rubenvp8510 force-pushed the token-propagation branch from d22bdfd to 7fd5aa4 Compare January 14, 2020 15:08

Add query service token propagation support

750e826

Signed-off-by: Ruben Vargas <[email protected]>

rubenvp8510 force-pushed the token-propagation branch 2 times, most recently from 7100f85 to 512ee05 Compare January 14, 2020 18:56

E2E Tests for token propagation

853b677

Signed-off-by: Ruben Vargas <[email protected]>

rubenvp8510 force-pushed the token-propagation branch 3 times, most recently from dc777fc to 25d8e83 Compare January 14, 2020 20:04

Use auth delegation to avoid oauth form parsing

f100fc9

Signed-off-by: Ruben Vargas <[email protected]> Signed-off-by: Ruben Vargas <[email protected]>

rubenvp8510 force-pushed the token-propagation branch from 25d8e83 to f100fc9 Compare January 14, 2020 20:50

rubenvp8510 requested a review from jpkrohling January 15, 2020 20:55

jpkrohling approved these changes Jan 16, 2020

View reviewed changes

kevinearls suggested changes Jan 16, 2020

View reviewed changes

rubenvp8510 added 2 commits January 16, 2020 15:26

E2E token test more reliable

233cfd4

Signed-off-by: Ruben Vargas <[email protected]>

Allow override es.archive.tls

93df3af

Signed-off-by: Ruben Vargas <[email protected]>

rubenvp8510 force-pushed the token-propagation branch from 7d1c1fc to 93df3af Compare January 16, 2020 21:30

Address formating comments

0bb2711

Signed-off-by: Ruben Vargas <[email protected]>

rubenvp8510 force-pushed the token-propagation branch from 1767c54 to 0bb2711 Compare January 16, 2020 21:34

rubenvp8510 requested a review from kevinearls January 17, 2020 15:42

kevinearls approved these changes Jan 22, 2020

View reviewed changes

jpkrohling merged commit c6fe438 into jaegertracing:master Jan 22, 2020


		client, err := oAuthAuthorization(queryHost, "user-test-token", "any")

		require.NoError(t, err)


		client, err := oAuthAuthorization(host, username, password)

		err = wait.Poll(retryInterval, timeout, func() (done bool, err error) {

Token propagation E2E tests. #581

Token propagation E2E tests. #581

Conversation

rubenvp8510 commented Aug 6, 2019 • edited Loading

rubenvp8510 commented Oct 10, 2019

jpkrohling commented Oct 14, 2019

rubenvp8510 commented Oct 14, 2019

rubenvp8510 commented Oct 15, 2019

jpkrohling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinearls left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpkrohling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpkrohling commented Jan 16, 2020

kevinearls commented Jan 16, 2020

kevinearls commented Jan 16, 2020

rubenvp8510 commented Jan 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinearls commented Jan 16, 2020

rubenvp8510 commented Jan 16, 2020

kevinearls left a comment

Choose a reason for hiding this comment

jpkrohling commented Jan 22, 2020

rubenvp8510 commented Aug 6, 2019 •

edited

Loading

rubenvp8510 commented Jan 16, 2020 •

edited

Loading